-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add missing theme sub args #6492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing theme sub args #6492
Conversation
subtheme(find_args(), "axis.", ".x") | ||
} | ||
|
||
#' @export | ||
#' @describeIn subtheme Theme specification for both y axes. | ||
theme_sub_axis_y <- function(title, text, ticks, ticks.length, line) { | ||
theme_sub_axis_y <- function(title, text, ticks, ticks.length, line, minor.ticks.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not minor.ticks
argument for this and above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no theme(axis.minor.ticks)
, they all descent from the major ticks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then why do you have them below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should've been more specific: they descent from position-resolved major ticks. To illustrate: there is no minor.ticks.y
because minor.ticks.y.left
and minor.ticks.y.right
descent from ticks.y.left
and ticks.y.right
respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find this weird but I trust your judgement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't disagree with you, it is just the least-worst option I think.
We deliberated in #5287 (comment) wether inheritance should be like this:
axis.ticks
/ \
axis.ticks.x axis.ticks.minor
| |
axis.ticks.x.top axis.ticks.minor.x
|
axis.ticks.minor.x.top
or like this:
axis.ticks
|
axis.ticks.x
|
axis.ticks.x.top
|
axis.ticks.x.top.minor
Both are valid but eventually we chose option 2, with the minor ticks inheriting from the most specific major ticks,
That is why there is no axis.ticks.x.minor
and axis.ticks.minor
theme elements.
I'm happy to entertain alternative parentage for the minor ticks, but that is something we can delay until after release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the review! |
This PR aims to fix #6491.
It integrates the
panel.widths
,panel.heights
,axis.minor.ticks.length.{...}
andaxis.minor.ticks.{...}
settings in thetheme_sub_axis_*()
family andtheme_sub_panel()
function.A demo:
Created on 2025-06-02 with reprex v2.1.1